Skip to content

ENH: support nanoseconds in Period constructor #38175

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 13 commits into from
Dec 2, 2020
Merged

ENH: support nanoseconds in Period constructor #38175

merged 13 commits into from
Dec 2, 2020

Conversation

arw2019
Copy link
Member

@arw2019 arw2019 commented Nov 30, 2020

Picking up #34720

ASVs:

asv continuous -f 1.1 upstream/master HEAD -b ^period

       before           after         ratio
     [3f510600]       [2b2f1d04]
     <GH34621-nanos-for-Period^2>       <GH34621-nanos-for-Period>
+      7.46±0.2ms       11.6±0.1ms     1.55  period.PeriodIndexConstructor.time_from_ints('D', False)
+      7.57±0.3ms      11.4±0.03ms     1.51  period.PeriodIndexConstructor.time_from_ints('D', True)
+      52.8±0.6ms       59.2±0.4ms     1.12  period.PeriodIndexConstructor.time_from_ints_daily('D', True)
+      52.9±0.9ms       59.0±0.1ms     1.11  period.PeriodIndexConstructor.time_from_ints_daily('D', False)
-      3.21±0.2μs      2.90±0.02μs     0.90  period.Indexing.time_get_loc

SOME BENCHMARKS HAVE CHANGED SIGNIFICANTLY.
PERFORMANCE DECREASED.

cc @jreback #34720 (comment) in case we want this for 1.2

@arw2019 arw2019 mentioned this pull request Nov 30, 2020
Closed
2 tasks
@arw2019 arw2019 added Period Period data type Needs Review labels Nov 30, 2020
@jreback jreback added this to the 1.2 milestone Dec 2, 2020
jreback
jreback requested changes Dec 2, 2020
View reviewed changes
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. can you add a whatsnew in bug fixes for 1.2

cc @jbrockmendel

@jreback jreback merged commit efbcd68 into pandas-dev:master Dec 2, 2020
@jreback
Copy link
Contributor

jreback commented Dec 2, 2020

thanks @arw2019

jorisvandenbossche
jorisvandenbossche reviewed Dec 4, 2020
View reviewed changes
pandas/_libs/tslibs/period.pyx
Comment on lines 2397 to +2399
dt, reso = parse_time_string(value, freq)
try:
ts = Timestamp(value)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't we effectively doing the parsing twice, this way? (once in parse_time_string, once in Timestamp) Any way to avoid this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes - @jreback suggested integrating this a bit more into parse_time_string

I opened #38312 to track resolution (I'll have a go at it if no one else does)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@arw2019 arw2019 mentioned this pull request Dec 5, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jorisvandenbossche jorisvandenbossche jorisvandenbossche left review comments

@jreback jreback jreback approved these changes

Assignees
No one assigned
Labels
Period Period data type
Projects
None yet
Milestone
1.2
Development

Successfully merging this pull request may close these issues.

BUG: Period constructor does not take into account nanonseconds BUG: Period with nanoseconds (?)
3 participants
@arw2019 @jreback @jorisvandenbossche